Use llvm-symbolizer's JSON output to provide function start lines #891

insilications · 2024-09-01T15:43:33Z

When analyzing a perf.data profile converted automatically via perf_to_profile via pprof -raw perf.data, no function start lines (s=0) are present in any of the locations. With 813a5fb, this can be easily solved by using the same JSON frame data from llvm-symbolizer to provide StartLine for Function.start_line. This solves #823.

When analyzing a perf.data profile converted automatically via perf_to_profile via pprof -raw perf.data, no function start lines (s=0) are present in any of the locations. With google@813a5fb, this can be easily solved by using the same JSON frame data from llvm-symbolizer to provide StartLine for Function.start_line. This solves google#823.

codecov-commenter · 2024-09-10T15:14:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.91%. Comparing base (0ed6a68) to head (27c179c).
Report is 42 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
+ Coverage   66.86%   66.91%   +0.05%     
==========================================
  Files          44       44              
  Lines        9824     9857      +33     
==========================================
+ Hits         6569     6596      +27     
- Misses       2794     2820      +26     
+ Partials      461      441      -20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aalexand · 2024-09-10T17:01:12Z

A few CI tests failed:

++ go test github.com/google/pprof/fuzz github.com/google/pprof/internal/binutils github.com/google/pprof/internal/driver github.com/google/pprof/internal/elfexec github.com/google/pprof/internal/graph github.com/google/pprof/internal/measurement github.com/google/pprof/internal/report github.com/google/pprof/internal/symbolizer github.com/google/pprof/internal/symbolz github.com/google/pprof/profile
ok  	github.com/google/pprof/fuzz	0.003s
--- FAIL: TestObjFile (0.80s)
    --- FAIL: TestObjFile/fixed_load_address (0.67s)
        binutils_test.go:379: SourceLine for main: got [{main /tmp/hello.c 3 0 0}]; want [{main /tmp/hello.c 3 0 3}]
    --- FAIL: TestObjFile/simulated_ASLR_address (0.13s)
        binutils_test.go:379: SourceLine for main: got [{main /tmp/hello.c 3 0 0}]; want [{main /tmp/hello.c 3 0 3}]
FAIL
FAIL	github.com/google/pprof/internal/binutils	0.860s

insilications · 2024-09-11T10:09:47Z

Fixed TestPEFile windows tests that were failing and were obviously wrong. I have no idea why TestObjFile linux tests were failing in the CI tests. Let's try again please?

aalexand · 2024-09-12T15:32:11Z

Looks like the CI still failed.

aalexand · 2024-09-12T15:33:51Z

Maybe the CI setup needs to install llvm-symbolizer.

insilications · 2024-09-13T08:59:28Z

Maybe the CI setup needs to install llvm-symbolizer.

Could be... The TestPEFile windows tests were fixed because ci / test-windows is not failing anymore. I have no idea why it is failing on Linux. In my Linux system I even added a temporary debug log to double check: t.Logf("DEBUG SourceLine for main: got %v; want %v\n", gotFrames, wantFrames):

=== RUN   TestObjFile
=== RUN   TestObjFile/fixed_load_address
    binutils_test.go:378: DEBUG SourceLine for main: got [{main /tmp/hello.c 3 0 3}]; want [{main /tmp/hello.c 3 0 3}]
=== RUN   TestObjFile/simulated_ASLR_address
    binutils_test.go:378: DEBUG SourceLine for main: got [{main /tmp/hello.c 3 0 3}]; want [{main /tmp/hello.c 3 0 3}]
--- PASS: TestObjFile (0.10s)
    --- PASS: TestObjFile/fixed_load_address (0.09s)
    --- PASS: TestObjFile/simulated_ASLR_address (0.01s)

I wish we could solve this because it would close #823

aalexand · 2024-09-15T23:35:06Z

Could be... The TestPEFile windows tests were fixed because ci / test-windows is not failing anymore. I have no idea why it is failing on Linux. In my Linux system I even added a temporary debug log to double check: t.Logf("DEBUG SourceLine for main: got %v; want %v\n", gotFrames, wantFrames):

binutils.go falls back to nm or binutils' addr2line if llvm-symbolizer is not available. I think this is what might be happening since perhaps the CI environment doesn't have llvm-symbolizer. I suspect we need to figure out how to install llvm-symbolizer around this line:

pprof/.github/workflows/ci.yaml

Line 149 in a0b0bb1

sudo apt-get install graphviz

…tests Modify the `Fetch dependencies` step in `test-linux` job to include the installation of llvm. Add `Verify llvm-symbolizer installation` step to ensure that llvm-symbolizer is available in the CI environment for the Linux tests. This should resolve the test failures related to the missing llvm-symbolizer tool.

insilications · 2024-09-16T10:44:40Z

binutils.go falls back to nm or binutils' addr2line if llvm-symbolizer is not available. I think this is what might be happening since perhaps the CI environment doesn't have llvm-symbolizer. I suspect we need to figure out how to install llvm-symbolizer around this line:

pprof/.github/workflows/ci.yaml

Line 149 in a0b0bb1

sudo apt-get install graphviz

I have pushed changes to ci.yaml to attempt to fix it. Lets check if it passes now?

insilications · 2024-09-16T11:02:00Z

I ran the CI workflow on my fork. It is now passing all test-linux jobs on ubuntu-22.04. But it's failing on ubuntu-20.04. My added Verify llvm-symbolizer installation step shows that llvm-symbolizer --version on ubuntu-22.04 is:

llvm-symbolizer
Ubuntu LLVM version 14.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: znver3

But on ubuntu-20.04 is:

LLVM (http://llvm.org/):
  LLVM version 10.0.0
  
  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: (unknown)

Enhances the CI workflow to ensure proper installation of LLVM and the `llvm-symbolizer` across both Ubuntu 20.04 and 22.04 environments. These changes address the issue of an outdated llvm-symbolizer on Ubuntu 20.04, while maintaining compatibility with Ubuntu 22.04. - Add LLVM official repository for Ubuntu 20.04 - Install LLVM 14 on Ubuntu 20.04 to ensure a recent version - Use update-alternatives to manage llvm-symbolizer versions - Maintain existing LLVM 14 for Ubuntu 22.04

insilications · 2024-09-16T11:23:03Z

Last commit addresses the issue of an outdated llvm-symbolizer on Ubuntu 20.04. It is now passing all tests.

aalexand · 2024-09-25T18:10:58Z

Sorry it's taking me a while to review this. I've been staring at the code on and off, I'm just not a fan of how much this complicates the CI configuration with this code to get llvm/clang installed. But I guess there is no better way so I should just bite the bullet and LGTM.

insilications · 2024-09-25T18:40:30Z

Sorry it's taking me a while to review this. I've been staring at the code on and off, I'm just not a fan of how much this complicates the CI configuration with this code to get llvm/clang installed. But I guess there is no better way so I should just bite the bullet and LGTM.

Not a problem at all. Take the time you need to give it a thorough review.

Yeah, the additional complexity is sad but it is required... Ubuntu 20.04 provides LLVM 10's llvm-symbolizer, which is very old and does not produce a JSON data frame with StartLine. The alternative would be to add a minimum version check (LLVM 14) before the test that uses llvm-symbolizer and skip it:

pprof/internal/binutils/binutils_test.go

Line 336 in a0b0bb1

skipUnlessLinuxAmd64(t)

.github/workflows/ci.yaml

aalexand previously approved these changes Sep 10, 2024

View reviewed changes

Merge branch 'main' into llvm-symbolizer-startline

659b4d1

Fixed TestPEFile windows test

8dda08c

insilications dismissed aalexand’s stale review via 8dda08c September 11, 2024 10:07

Fixed TestMachoFiles mac tests

df836a4

Merge branch 'main' into llvm-symbolizer-startline

b3032ec

aalexand requested changes Sep 27, 2024

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

insilications and others added 2 commits September 29, 2024 06:53

ci: Make it clear that our goal is to install llvm-symbolizer

6ad3ca5

Merge branch 'main' into llvm-symbolizer-startline

27c179c

aalexand approved these changes Sep 29, 2024

View reviewed changes

aalexand merged commit 255acd7 into google:main Sep 29, 2024
31 checks passed

aalexand mentioned this pull request Oct 18, 2024

"go test ./..." fails when llvm-symbolizer is not available (even if llvm-symbolizer-14 is there) #906

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use llvm-symbolizer's JSON output to provide function start lines #891

Use llvm-symbolizer's JSON output to provide function start lines #891

insilications commented Sep 1, 2024

codecov-commenter commented Sep 10, 2024 •

edited

Loading

aalexand commented Sep 10, 2024

insilications commented Sep 11, 2024 •

edited

Loading

aalexand commented Sep 12, 2024

aalexand commented Sep 12, 2024

insilications commented Sep 13, 2024

aalexand commented Sep 15, 2024

insilications commented Sep 16, 2024

insilications commented Sep 16, 2024

insilications commented Sep 16, 2024

aalexand commented Sep 25, 2024

insilications commented Sep 25, 2024

Use llvm-symbolizer's JSON output to provide function start lines #891

Use llvm-symbolizer's JSON output to provide function start lines #891

Conversation

insilications commented Sep 1, 2024

codecov-commenter commented Sep 10, 2024 • edited Loading

Codecov Report

aalexand commented Sep 10, 2024

insilications commented Sep 11, 2024 • edited Loading

aalexand commented Sep 12, 2024

aalexand commented Sep 12, 2024

insilications commented Sep 13, 2024

aalexand commented Sep 15, 2024

insilications commented Sep 16, 2024

insilications commented Sep 16, 2024

insilications commented Sep 16, 2024

aalexand commented Sep 25, 2024

insilications commented Sep 25, 2024

codecov-commenter commented Sep 10, 2024 •

edited

Loading

insilications commented Sep 11, 2024 •

edited

Loading